-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[native_assets_builder] Use package:file
s FileSystem
abstraction
#1825
Conversation
59f08d1
to
1ee0111
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please mention #90 in the PR description.
Unfortunately, almost of our tests also require process runs, so we can't change any of our tests to use the virtual file system.
Anyway, this is a step in that direction, so LGTM!
There's in more import in pkgs/native_assets_builder/lib/src/utils/run_process.dart which can get a show xxx
to ensure we don't see any of the dart:io
File/Directory types anywhere in lib/src.
pkgs/native_assets_builder/lib/src/dependencies_hash_file/dependencies_hash_file.dart
Show resolved
Hide resolved
pkgs/native_assets_builder/lib/src/package_layout/package_layout.dart
Outdated
Show resolved
Hide resolved
pkgs/native_assets_builder/lib/src/package_layout/package_layout.dart
Outdated
Show resolved
Hide resolved
Done.
One step at a time.
Done. |
Thanks @mkustermann ! |
The main motivation for this change is to make
package:native_assets_builder
cleanly usable in flutter tools. Right now flutter tools operates on aFileSystem
and checks whether.dart_tool/package_config.json
exists using thatFileSystem
but then reads the file with this packagag's code which usesdart:io
.This inconsistency causes issues in flutter tools. Using
package:file
should solve this and would alsoallow injecting mocks. To be fully mockable the implementation would also need to make process invocations
mockable via
package:process
. That's a future task.See also #90